This repository was archived by the owner on Jan 16, 2025. It is now read-only.
Add server side pagination #1673
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JSReds
force-pushed
the
add-server-side-pagination
branch
2 times, most recently
from
2df257f to
222d7f6
Compare
JSReds
force-pushed
the
add-server-side-pagination
branch
2 times, most recently
from
30b39b4 to
fbea07a
Compare
thgreasi
reviewed
Dec 9, 2022
| @@ -186,10 +193,12 @@ export class TableBase<T extends {}> extends React.Component< | |||
| this.setRowSelection(checkedItems); | |||
There was a problem hiding this comment.
I think we also need to doublecheck how selection works.
- We need to update
toggleChecked, since it does include a client side sorting and assumes it has all the data while trying to map the checkbox the user clicked and the item that the row was about. - We probably also need to check
toggleAllCheckedas well, and maybe in server side more it should be returningall, to let the consumer know that they need to query and fetch AAAAAALLLLLLLLLL item IDs from the backend.
There was a problem hiding this comment.
Work in progress. I need to merge this PR first to be able to do something like this in the following PR:
isDisabled & actionFn:
- isServerSide: false => T[]
- isServerSide: true =>
- selecting N items: { id: { $in: [] } }
- select-all: { $and: [{ belongs_to__application: xyz }, searchFilters] }
JSReds
force-pushed
the
add-server-side-pagination
branch
2 times, most recently
from
422d9d0 to
b307ce4
Compare
thgreasi
reviewed
Jan 2, 2023
src/components/Table/TableBase.tsx
Outdated
| const totalItems = items.length; | ||
| const _itemsPerPage = itemsPerPage || 50; | ||
| const totalItems = serverSide ? pagination?.totalItems : items.length; | ||
| const itemsPerPage = pagination?.itemsPerPage || DEFAULT_ITEMS_PER_PAGE; |
There was a problem hiding this comment.
Given that this is a minor & itemsPerPage is still in the properties, we should
Suggested change
| const itemsPerPage = pagination?.itemsPerPage || DEFAULT_ITEMS_PER_PAGE; | |
| const itemsPerPage = pagination?.itemsPerPage ?? this.props.itemsPerPage ?? DEFAULT_ITEMS_PER_PAGE; |
both here and in setPage.
Otherwise we can do a major and remove it completely.
I think atm I'm in favor of the minor so that we can do a single major after the product call on how selection should work.
Change-type: minor Signed-off-by: Andrea Rosci <[email protected]>
JSReds
force-pushed
the
add-server-side-pagination
branch
from
b307ce4 to
667f21a
Compare
Change-type: patch Signed-off-by: Andrea Rosci <[email protected]>
JSReds
force-pushed
the
add-server-side-pagination
branch
from
667f21a to
f169c31
Compare
thgreasi
approved these changes
Jan 2, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Connects-to: #
See:
Depends-on:
Change-type: major|minor|patch
Contributor checklist
npm run generate-snapshotsReviewer Guidelines